-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow stdClass in XML responses #38745
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be tested in \Test\AppFramework\Middleware\BaseResponseTest
?
2a75103
to
587ad1b
Compare
Signed-off-by: jld3103 <jld3103yt@gmail.com>
587ad1b
to
7f46516
Compare
…class Allow stdClass in XML responses Signed-off-by: Silke Suck <silke.suck@adacor.com>
Many thanks for the PR. A backport to Nextcloud 27.1.0 would be desirable. |
@AlexM4H does it fix a bug for you? This was only meant as a feature, so it doesn't get any backport |
On a technical level this is still a bug with the respective apps and not the core as this was never supported. @ChristophWurst @come-nc shall we backport it anyway? |
Hard to say. Backporting API change is always dangerous as it may trigger subtle sideeffects. Without this an stdClass is just stripped from XML output? @provokateurin I think you’re more knowledgeable than me to decide whether to backport. |
It's not breaking at all, but if an app returns stdClass the output without this patch would just be empty thus broken. I still feel like the apps should fix it themselves as any version before this patch is broken. That would mean apps can't target <28 if they rely on this. |
Please keep in mind that Forms is a featured app. "Featured apps are developed by and in the community. They provide core functionality and are ready for production use." The fix would be necessary for some app developers for NC 26 / 27, as NC 28 already includes the fix as a new feature. |
What would we need to do to fix the problem in Forms? |
The corresponding issue is still open |
@AlexM4H yes, I know. We're waiting for a server fix there 😆 My question was for @provokateurin 😉 |
@AlexM4H Sorry, this is not helpful. This is a bug with the app and not the core. As I said the behavior the forms app is relying on was never intended to work (in previous versions). I added this as a feature but it is only available in >=28. If you target 28 and higher you can surely rely on it.
@Chartman123 For example Talk also had to work around it like this: https://github.com/nextcloud/spreed/blob/eaf04bbe5a7626cc1565cc455124f8a10b2ff078/lib/Model/Message.php#L163 |
Thanks I'll have a look at it :) |
In the future when you target >=28 you can of course drop the workaround. |
Summary
Allows returning
\stdClass()
in DataResponse that are encoded to XML.JSON can already to it and converts it to
{}
, but for XML special handling is required.This change is required in order to properly describe APIs using empty JSON objects (
[]
gets converted to[]
in JSON instead of{}
). XML just also needs to support it.Checklist